-
Notifications
You must be signed in to change notification settings - Fork 10
Add rule to enforce importing components with sx prop from @primer/styled-react #382
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
🦋 Changeset detectedLatest commit: 2dcde64 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
fd1cff5
to
767e170
Compare
… for components without sx prop
…ith and without sx prop, including aliasing for conflicts
…ents in imports, handling conflicts and aliasing for components used with and without sx prop
…solidate styled-react imports into a single statement with aliasing support
…or styled-react imports and improve error reporting for components used without sx prop
I did a test migration on github here https://github.com/github/github/pull/395613 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds a new ESLint rule use-styled-react-import
that enforces correct import locations for Primer React components based on whether they use the sx
prop. The rule ensures components using sx
are imported from @primer/styled-react
while components without sx
remain in @primer/react
.
- Implements automatic fixing for import statements and JSX element names
- Handles complex scenarios like alias imports when components are used both with and without
sx
props - Provides comprehensive test coverage for various import patterns and edge cases
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
File | Description |
---|---|
src/rules/use-styled-react-import.js |
Core rule implementation with logic for tracking component usage and automatically fixing imports |
src/rules/__tests__/use-styled-react-import.test.js |
Comprehensive test suite covering valid/invalid cases and auto-fix behavior |
src/index.js |
Exports the new rule making it available to consumers |
docs/rules/use-styled-react-import.md |
Documentation explaining rule purpose, examples, and usage guidelines |
.changeset/smart-rocks-fail.md |
Changeset file for release management |
|
||
// If this is the only import, replace the whole import | ||
if (otherSpecifiers.length === 0) { | ||
const prefix = styledTypes.has(importName) ? 'type ' : '' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The type
prefix is added for TypeScript type imports, but the rule doesn't check if the file is TypeScript or if the import was originally a type import. This could break JavaScript files or incorrectly convert regular imports to type imports.
const prefix = styledTypes.has(importName) ? 'type ' : '' | |
const isTypeScript = isTypeScriptFile(context.getFilename()); | |
const isTypeImport = specifier.importKind === 'type'; | |
const prefix = (isTypeScript && isTypeImport) ? 'type ' : ''; |
Copilot uses AI. Check for mistakes.
} | ||
|
||
// Add new import | ||
const prefix = styledTypes.has(importName) ? 'type ' : '' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same issue as above - the type
prefix is unconditionally added for type imports without checking the original import syntax or file type.
const prefix = styledTypes.has(importName) ? 'type ' : '' | |
const isTypeImport = specifier.importKind === 'type' | |
const isTSFile = context.getFilename().endsWith('.ts') || context.getFilename().endsWith('.tsx') | |
const prefix = isTypeImport && isTSFile ? 'type ' : '' |
Copilot uses AI. Check for mistakes.
|
||
if (!isFirstComponent) { | ||
return null | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic for determining the 'first component' is complex and fragile. The condition checks different arrays and could lead to multiple fixes being applied for the same import node. Consider using a more explicit flag or state tracking mechanism.
} | |
// Only apply the fix once per import node using an explicit flag | |
if (changes.fixed) { | |
return null | |
} | |
changes.fixed = true; |
Copilot uses AI. Check for mistakes.
} | ||
|
||
// Only apply the fix once per import node (for the first component processed) | ||
const isFirstComponent = changes.toMove[0] === componentName |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This simpler version of the 'first component' check is inconsistent with the more complex version above (lines 185-188). The inconsistency makes the code harder to understand and maintain.
const isFirstComponent = changes.toMove[0] === componentName | |
const isFirstComponent = changes.toMove.find(name => name === componentName) === changes.toMove[0] |
Copilot uses AI. Check for mistakes.
Closes https://github.com/github/primer/issues/5613
use-styled-react-import
💼 This rule is disabled in the ✅
recommended
config.🔧 This rule is automatically fixable by the
--fix
CLI option.Enforce importing components that use
sx
prop from@primer/styled-react
instead of@primer/react
, and vice versa.Rule Details
This rule detects when certain Primer React components are used with the
sx
prop and ensures they are imported from the temporary@primer/styled-react
package instead of@primer/react
. When the same components are used without thesx
prop, it ensures they are imported from@primer/react
instead of@primer/styled-react
.When a component is used both with and without the
sx
prop in the same file, the rule will import the styled version with an alias (e.g.,StyledButton
) and update the JSX usage accordingly to avoid naming conflicts.It also moves certain types and utilities to the styled-react package.
Components that should be imported from
@primer/styled-react
when used withsx
:Types and utilities that should always be imported from
@primer/styled-react
:BoxProps
(type)SxProp
(type)BetterSystemStyleObject
(type)sx
(utility)Examples
❌ Incorrect
✅ Correct
Options
This rule has no options.
When Not To Use It
This rule is specifically for migrating components that use the
sx
prop to the temporary@primer/styled-react
package. If you're not using thesx
prop or not participating in this migration, you can disable this rule.